Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

USB sync to drive: enhanced services_start / services_stop scripts to… #2

Closed
wants to merge 1 commit into from

Conversation

jacques42
Copy link

@jacques42 jacques42 commented Oct 14, 2020

... be more robust.

Thanks for checking. How could I miss that - I think I actually never tested the core use-case of taking a picture omg. I did some additions to not only check for the PID file as such but have the script be a bit more robust - both before starting service but also when stopping services.

And one small tweak in sync-to-drive.js ---- when writing the PID to file, I believe it was meant to be 'flags' instead of 'mode'. And I would prefer mode of 'w' to overwrite an existing, possibly stale log file. 'wx' would force the process to exit when the file exists. but eventually not the actual process itself.

andi34#115

@jacques42 jacques42 mentioned this pull request Oct 14, 2020
2 tasks
@thatonedude3470
Copy link
Owner

thatonedude3470 commented Oct 15, 2020

Thanks for another PR. While you did this I also thought about how to solve this the most elegant way and came up with following ideas:

  • In the script add listeners to events like SIGINT and SIGTERM so when PHP sends the kill signal the scripts begins to unmount all the drives it mounted and then deletes its own PID file before exiting

  • Add a file watch to the scripts PID and when the file stops to exists quit and unmount

  • Inside the script look if another node instance is running the same file already (check pgrep for its own name) and quit if its running already

I'm leaning towards the first and last option. Last option to prevent it running multiple times and event listeners to unmount before quitting.
Only problem I see is that the script could begin to unmount while rsync is still running, so it would have to wait until rsync is done, then unmount and only after that the script could be restarted by PHP, but that call happens often.
In addition the user cannot tell whether they can pull the drive.
So there is still some stuff to figure out :D

Could you change the posix_kill($procPID, 9) inside the services_stop.php to posix_kill($procPID, $sigType) so it becomes configurable because one can't listen to an SIGKILL

@jacques42
Copy link
Author

Love it - this is of course more clean. And I agree the combination of 1+3 sounds preferred.

JavaScript is single-threaded afaik. So assume the kill signal processing passes to the handler and hence is entered into the event queue, rsync will always finish first. But I am not sure about the exact behavior for handling system signals in node.js and scripts.

Another approach could be to use a semaphore around rsync execution, which can be evaluated by the signal handler.

Let me know how I can help and of course feel free to adjust the stop script the way you need it.

@thatonedude3470
Copy link
Owner

... queue, rsync will always finish first.

Sadly no, I've built the script to detach the rsync shells from itself to not wait until it finishes before starting the next rsync (when there are more targets to rsync to).

Maybe we could convert this part to Promises?
Then it could go like this: Start -> [...] -> Create rsync promises -> Wait until they're all done (Promise.allSettled) -> next loop

I've done the cleanup part, sadly I couldn't figure out the wait part to wait (e.g. 30 seconds) before killing still running rsync processes.
Now the script just looks if rsync is running for the current user and kills it if so.

Do you know how I can push to your branch, so we can fix everything in this PR before I merge it.

@jacques42
Copy link
Author

Hey - I've been offline for a while, sorry for that.

Is there commits I can pull into my branch, so it becomes part of the PR ? Or can you accept my PR into yours and we continue to build from there ? Admit I am not the git expert... If all that does not work I could give you access to my repo.

I'd try use 'setTimeout()' for a 30sec wait - but I have not looked into the code yet.

On the other side, if the rsync runs every say 30 seconds to 1 minute , there is little to sync every single time. So I would assume that in a real photobox use-case, until one eventually shuts down the rsync script via config change, the box is not active (active as being currently used in an event) and the likelyhood of missing pictures copied on the USB stick during rsync is low. So thinking about that, I believe if we chose to hard stop any possibly active rsync and then cleanly unmount the USB device should bear low risk to miss files. And if then worst case one is missed from syncing to the stick, anyways the files are still on the box. Just looking at risk vs. effort.

Suggest we get all devs merged first and go from there ?

Hmm - and maybe we integrate some sort of status information about the rsync - i.e. count number of files on both sides or do some sort of diff - to show and feedback to users that all files have been copied to the USB stick. That way one could actually see that no picture is missed out ? Thinking aloud...

@andi34
Copy link

andi34 commented Oct 24, 2020

Hey and thanks to both of you for your work!
I've created a seperate repo for you, maybe you can together work there on it: https://github.com/andi34/photobooth-wip/commits/add-filesync

Once you're done @thatonedude3470, you can force push over your current add-filesync branch here so the PR to Photobooth get updated.

@andi34
Copy link

andi34 commented Oct 27, 2020

Forgot to mention @thatonedude3470 , you need to accept the invite to push to mentioned repo.

@jacques42
Copy link
Author

jacques42 commented Oct 27, 2020

I continue the work in https://github.com/andi34/photobooth-wip - see code changes there.

  • added a basic inter-process communication, so that the child process (rsync) will signal the parent (sync-to-file.js) upon termination (graceful or not).
  • added a semaphore approach to ensure one rsync instance running at a time. This replaces the need for isProcessRunning()
  • added a signal handler to gracefully ramp down and terminate upon SIGHUP, SIGTERM, SIGINT
  • upon signal, hard kill rsync after 60 sec waiting time
  • updated the service_stop.php script to send SIGTERM rather than SIGKILL

Todo:

Note:

  • for debugging reasons the current code executes a sleep 25 command in a shell, rather than an actual rsync command

Commit: 4eaab02

@thatonedude3470
Copy link
Owner

Sorry for my long inactivity, couldn't work on the go and forgot to notify you.

Thanks for all the help, I'll look into the changes and merge them after some testing :)

@thatonedude3470
Copy link
Owner

@jacques42 what IDE or editor are you using?

Okay so there is a lot to be done, just for reference I pushed the changes I couldn't commit into your git into the anotherfix branch.
I've done some fixes, but forgot to use branches, maybe we should start using branches so we don't conflict while working?

@jacques42
Copy link
Author

@jacques42 what IDE or editor are you using?

Plain good old emacs on SSH / TTY :-)

@jacques42
Copy link
Author

Hey - I took a look at the code in your branch, and understand why it does not merge. I think currently we have different ideas on how to approach this, which of course is fine but I do not want to get in your way here. This is your idea and script, hence you lead. That said, I will step back a bit and not continue develop my branch further - it does not make sense with both of us working the same thing in parallel but with different approaches.

Again - I do like the idea a lot and I am happy to help integrate to photobooth where I can. Yet I believe at this point it is a better approach if you lead the development on the sync script itself, and I support with intergrations to PB where I can.

Let me know if you view this differently but I think this is where we are right now.

@jacques42
Copy link
Author

Hey @thatonedude3470 - I wanted to check back with you, this work has been sitting stale for a while. I think that during the holiday break I'd find some time to finish the work. I like the feature and it should go to the master, it would be sad to see it sitting stale forever. Let me know what you think and whether you want to work on it at all. Or once I find time I'll give it a shot and try to finalize it into a functioning version for to include to the next release.

@jacques42 jacques42 closed this Dec 19, 2020
@jacques42 jacques42 deleted the branch thatonedude3470:add-filesync December 19, 2020 16:51
@jacques42 jacques42 deleted the add-filesync branch December 19, 2020 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants